Skip to content

Removing pathconfig warnings, defaulting the python path to include the local copy of the standard library #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10,000 commits into from

Conversation

zig-robotics
Copy link

@zig-robotics zig-robotics commented Jan 22, 2025

Thanks for making this available! I made a few tweaks that allow the produced binary to be used out of the box. This allows for this package to be used as part of the build system. It may be better to point the prefix paths to the standard library as then we don't need to suppress the pathconfig warnings, but that would require copying the directory into the expected python3.11 directory which isn't straight forward to do as part of the build as we can't use lazy paths as flags. I'm open to suggestions to improve this though. I use this to do code generation thats required for building the ROS project. Here's one of the few places this gets used. Inline python in your build feels pretty cursed but I'm impressed it was so easy to remove a system dependency on python.

https://github.com/zig-robotics/zigros/blob/v0.1.0/rcutils/build.zig#L48-L67

miss-islington and others added 30 commits April 7, 2023 21:38
…e cases (pythonGH-102813, pythonGH-103343)

(cherry picked from commit 4dc339b)

Co-authored-by: AN Long <[email protected]>
Co-authored-by: Barney Gale <[email protected]>
…thonGH-99150)

* Quote paths in os.spawn tests on Windows so they work with spaces

* Add NEWS entry for os spawn test fix

* Fix code style to avoid double negative in os.spawn tests

Co-authored-by: Jelle Zijlstra <[email protected]>

---------

(cherry picked from commit a34c796)

Co-authored-by: C.A.M. Gerlach <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
…ted APIs (pythonGH-103378)

Migrate `SSLContext.set_ecdh_curve()` not to use deprecated OpenSSL APIs.
(cherry picked from commit 3516704)

Co-authored-by: Dong-hee Na <[email protected]>
(cherry picked from commit 090e26e)

Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
…otatedAlias, Annotated}` (pythonGH-103405)

(cherry picked from commit dc604a8)

Co-authored-by: Nikita Sobolev <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Kirill <[email protected]>
) (python#103416)

pythongh-103059: Clarify gc.freeze documentation (pythonGH-103058)
(cherry picked from commit 8b1b171)

Co-authored-by: raylu <[email protected]>
(cherry picked from commit 4cd1cc8)

Co-authored-by: Zac Hatfield-Dodds <[email protected]>
…thonGH-103398)

(cherry picked from commit e071f00)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
(cherry picked from commit 449bf2a)

Co-authored-by: Tian Gao <[email protected]>
STRICT boundary:

- fix bitwise operations
- make default for Flag
(cherry picked from commit 2194071)

Co-authored-by: Ethan Furman <[email protected]>
…a type (pythonGH-103495) (pythonGH-103514)

a mixin must either have a __new__ method, or be a dataclass, to be interpreted as a data-type; an __init__ method is not enough (restores pre-3.11 behavior for non-dataclasses).

(cherry picked from commit a6f9594)
Co-authored-by: Ethan Furman <[email protected]>
…unused (pythonGH-103554)

(cherry picked from commit 4fe1c4b)

Co-authored-by: Nikita Sobolev <[email protected]>
…pythonGH-103414)

(cherry picked from commit d65ed69)

Co-authored-by: Furkan Onder <[email protected]>
Co-authored-by: Erlend E. Aasland
…st cases (pythonGH-103569)

(cherry picked from commit f39e00f)

Co-authored-by: Rafael Fontenelle <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
…docs (pythonGH-103586)

(cherry picked from commit f4d0879)

Co-authored-by: Nikita Sobolev <[email protected]>
There was an extra `>` in the url.
(cherry picked from commit ed206e3)

Co-authored-by: Alexander Ryabov <[email protected]>
…nce" guarantee (pythonGH-103669) (python#103682)

pythonGH-103475: cache() and lru_cache() do not have a "call once" guarantee (pythonGH-103669)
(cherry picked from commit e5eaac6)

Co-authored-by: Raymond Hettinger <[email protected]>
…Pygments 2.15.0" (pythonGH-103616) (python#103695)

Revert "Avoid error lexing multiprocessing docs code block on Pygments 2.15.0" (pythonGH-103616)

This reverts commit ace51dc.
(cherry picked from commit 8cb2b0f)

Co-authored-by: Hugo van Kemenade <[email protected]>
@zig-robotics zig-robotics changed the title Removing pathconfig warnings, defaulting the python path to include t… Removing pathconfig warnings, defaulting the python path to include the local copy of the standard library Jan 22, 2025
@andrewrk
Copy link
Collaborator

Hmmm, this would be OK if it were only running python at build time, but if your goal is to produce a python that works at runtime, it's desirable to set the pythonpath at runtime. I suppose that's already possible. Have you tried setting the respective environment variable instead of hard-coding it with this macro?

@zig-robotics
Copy link
Author

Thanks for the reply! Like you mention this doesn't stop you from specifying additional python paths at run time, though it dose dirty the default paths a bit. Here are some examples:

Path with original build

$ PYTHONPATH=./Lib ./zig-out/bin/cpython-original -c "import sys
print(sys.path)"
['', '/home/.../zig/cpython/Lib', '/usr/local/lib/python311.zip', '/usr/local/lib/python3.11', '/usr/local/lib/python3.11/lib-dynload']

Path with the proposed build, first with no PYTHONPATH env passed, then with the local lib passed and then with the system lib:

$ ./zig-out/bin/cpython -c "import sys
print(sys.path)"
['', '/usr/local/lib/python311.zip', '/home/.../zig/cpython/Lib', '/usr/local/lib/python3.11', '/usr/local/lib/python3.11/lib-dynload']
$ PYTHONPATH=./Lib ./zig-out/bin/cpython -c "import sys
print(sys.path)"
['', '/home/.../zig/cpython/Lib', '/usr/local/lib/python311.zip', '/usr/local/lib/python3.11', '/usr/local/lib/python3.11/lib-dynload']
$ PYTHONPATH=/usr/lib/python3.11/ ./zig-out/bin/cpython -c "import sys
print(sys.path)"
['', '/usr/lib/python3.11', '/usr/local/lib/python311.zip', '/home/.../zig/cpython/Lib', '/usr/local/lib/python3.11', '/usr/local/lib/python3.11/lib-dynload']

The issue I ran into with trying to set the environment variable on the run step is that the Lib directory is only accessible through a LazyPath, and there's no way to set environment variables with a LazyPath. It is possible to resolve this particular lazy path during the configure phase since we know it's not a generated path in this case, but that felt like abusing the LazyPath a bit. This same LazyPath thing is true when hardcoding the macro as well, but I thought b.path().getPath(b) was a little more obvious that it was okay to do despite the "Intended for use during the make phase only" warning that getPath comes with. At the time I thought it made more sense to me to address this in the python package as that makes the cpython artifact directly usable without this implicit requirement on pulling in lib, but I can see your side that this is really only a requirement if using python in the build system. The following does work:

const std = @import("std");

pub fn build(b: *std.Build) void {
    const python_command =
        \\import sys
        \\print(sys.path)
    ;
    const python_dep = b.dependency("python", .{});
    
    var python_step = b.addRunArtifact(python_dep.artifact("cpython"));
    python_step.setEnvironmentVariable("PYTHONPATH", python_dep.path("Lib").getPath(b));
    python_step.addArgs(&.{ "-c", python_command });
    
    b.getInstallStep().dependOn(&python_step.step);
}

Additionally in other areas of my project I wrap the underlying python running in a zig program to more easily get around generated LazyPath issues (among other things). This lets you pass python paths as actual LazyPaths which is required for some python dependencies that the project generates itself.
https://github.com/zig-robotics/zigros/blob/v0.1.0/rosidl/src/adapter_generator.zig#L41

Would moving all the python running to a zig wrapper be a more idiomatic approach?

In either case, are you okay with disabling the pathconfig warnings? The warning shows up in the build output even though there's no real issue (this is true with the runtime use case as well unless there happens to be a python3.11 directory in /usr/local)

@andrewrk
Copy link
Collaborator

andrewrk commented Jan 22, 2025

I don't mind disabling those warnings, but it would be good to try and see if there is a different approach first before patching. The more patches we have the more effort it takes to pull in upstream changes, and the more work it takes possible users to decide whether it's OK to use and trust this package or not.

Edit: I didn't read your whole message yet, will come back to it later

andrewrk and others added 2 commits March 7, 2025 13:18
…he local copy of the standard library. This allows for this package to be used in build steps easily. Note that it may be better to point the prefix paths to the standard library as then we don't need to suppress the pathconfig warnings, but that would require copying the directory into the expected python3.11 directory which isn't straight forward to do as part of the build as we can't use lazy paths as flags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.